Preservation bugfix to put PreserveExpiryTime whenever machine transitions to Failed phase by r4mek · Pull Request #1147 · gardener/machine-controller-manager

r4mek · 2026-09-01T09:59:00Z

What this PR does / why we need it:

Now we add the PreserveExpiryTime on the machine if the machine has annotation: node.machine.sapcloud.io/preserve: "when-failed" and it transitions to Failed phase.
For auto-preservation flow, we add the PreserveExpiryTime on the machine if the failed machine gets the annotation: node.machine.sapcloud.io/preserve: "auto-preserved"
Miscellaneous code fix:
- Ensure objects retrieved from the lister cache are never mutated directly.
- Machine fetch in UpdateMachineWithRetries() from the lister cache, which may return a stale object.
- On update failure, requeue the object for the next reconciliation rather than retrying with a potentially stale cached value.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:
All the miscellaneous code fix are much needed to fix the aforementioned bug without surfacing/introducing new bugs.

Release note:

Fixed the scenario where a machine to be preserved can for brief moment not have PreserveExpiryTime set.

New flag --machine-preservation-timeout added to be used by mcs controller to set timeouts for auto preserved nodes

gardener-prow · 2026-09-01T09:59:05Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign takoverflow for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

r4mek · 2026-09-01T10:00:24Z

/kind bug

thiyyakat

Thanks for the changes and the refactoring! I haven't looked at the full PR yet. I had one case in mind for which I thought the new code may not behave as expected. Once you address that, I will look at the rest as well.

thiyyakat · 2026-09-03T07:17:34Z

 				defer trackers.Stop()
 				waitForCacheSync(stop, c)
-				err := c.updateMachineAndMachineDeploymentDeletionAnnotations(context.TODO(), testMachineDeployment)
+				err := func() error {


This may be a stupid question, but what is the point of the closure here? Can you not simply have just L2361?

thiyyakat · 2026-09-03T07:30:34Z

+			node.Annotations[PreserveMachineAnnotationKey]; ok &&
+			AllowedPreserveAnnotationValues.Has(val) {
+			return val, true


Please print a warning here if the value is not valid.

thiyyakat · 2026-09-03T07:32:38Z

+	if val, ok :=
+		machine.Annotations[PreserveMachineAnnotationKey]; ok &&
+		AllowedPreserveAnnotationValues.Has(val) {
+		return val, true


Please log a warning if the value is not valid.

thiyyakat · 2026-09-03T07:36:29Z

+	preserveAnnotationValue, exists := machineutils.GetPreserveAnnotationValue(node, machine)
+	if !exists {
 		return
-	} else if getErr != nil {
-		if !apierrors.IsNotFound(getErr) {
-			err = getErr
-			return
-		}
-		klog.Warningf("Couldn't find node %q for machine %q", nodeName, machine.Name)
-		err = nil
 	}


Please correct me if I'm wrong, but what about the case where preservation was being manipulated by only the machine object's annotations? If a machine was preserved, and the annotation was deleted to indicate that preservation should be stopped, you will end up returning early here instead of stopping preservation.

Good catch! Addressed in the latest commit ab6bf44

gagan16k

Thanks for the changes! Have some comments, PTAL

gagan16k · 2026-09-07T08:30:33Z

-				})
+			updatedMachine, err := machineutils.PatchMachine(ctx, c.Machines(machine.Namespace), &machine, func(m *v1alpha1.Machine) error {
+				m.Labels = labelsutil.AddLabel(m.Labels, v1alpha1.DefaultMachineDeploymentUniqueLabelKey, hash)
+				return nil


This will force a reconcile instead of retry on returning 409 errors, so the unit test for LDRCBST could flake even more now. Might need to find another way to correct that.

After our offline discussions, the error is very low and honestly I am not really sure why the LDRCBST test is failed. More investigation would be required for fixing it and not sure whether we want to tackle it now.

@aaronfern

gagan16k · 2026-09-07T09:46:24Z

+	// Deep-copy to avoid mutating cache objects downstream.
+	for i, m := range filteredMachines {
+		filteredMachines[i] = m.DeepCopy()
+	}
+


Great change, but I was thinking if we could apply this while creating the filteredMachines slices, in claimMachines().
Argument for: Saves extra work on the slice, and does it in the same loop while the slice is getting constructed.
Argument against: The other callers for the function, do not seem to write to the slice like this reconciler does, and thus do not really need a deep copy. WDYT?

I did not implement it in claimMachines() for the reason mentioned in Argument against. But if the team sees differently, I can implement it.

aaronfern

General feedback

Newly introduced flag should be mentioned in description and release notes
Error strings should not be capitalised
In general it's not nice that this huge bunch of miscellaneous code fixes are part of this bug fix. They are good pieces, but not required to fix the bug this PR talks about. It's fine for now as the work is done already, but keep this in mind for future reference

aaronfern · 2026-09-08T10:48:48Z

 	fs.Int32Var(&s.SafetyOptions.SafetyDown, "safety-down", s.SafetyOptions.SafetyDown, "Upper-limit minus safety-down value gives the lower-limit. This is the limits below which any temporarily frozen machineSet/machineDeployment object is unfrozen. lower-limit = desired + maxSurge (if applicable) + safetyUp - safetyDown.")

 	fs.DurationVar(&s.SafetyOptions.MachineSafetyOvershootingPeriod.Duration, "machine-safety-overshooting-period", s.SafetyOptions.MachineSafetyOvershootingPeriod.Duration, "Time period (in duration) used to poll for overshooting of machine objects backing a machineSet by safety controller.")
+	fs.DurationVar(&s.SafetyOptions.MachinePreserveTimeout.Duration, "machine-preserve-timeout", s.SafetyOptions.MachinePreserveTimeout.Duration, "Duration for which a failed machine should be preserved if it has the appropriate preserve annotation set.")


The issue description does not mention any new flag. A new flag like this is important enough to be mentioned as a release note

aaronfern · 2026-09-08T12:03:44Z

+			klog.Errorf("Error annotating and setting PreserveExpiryTime on machine %q for auto-preservation: %v", machine.Name, err)
+			return nil, err


Returning here stops patching of further machines. Consider pooling all error messages and returning an aggregate error at the end of the loop

aaronfern · 2026-09-09T02:34:02Z

+// nodeTemplateChanged returns true if machine's nodeTemplate is changed.
+func nodeTemplateChanged(machineset *v1alpha1.MachineSet, machine *v1alpha1.Machine) bool {


This func name is semantically incorrect. Generally a machine's nodeTemplate won't change but would rather get outdated and needs to be re-synced.
Consider names like nodeTemplateOutOfSync or machineNodeTemplateMismatch or feel free to choose another name that you feel is more accutare

aaronfern · 2026-09-09T02:35:23Z

+// configChanged returns true if machine's config is changed.
+func configChanged(machineset *v1alpha1.MachineSet, machine *v1alpha1.Machine) bool {


Same as above, name is not accurate

aaronfern · 2026-09-09T02:36:09Z

+// classKindChanged returns true if machine's class.Kind is changed.
+func classKindChanged(machineset *v1alpha1.MachineSet, machine *v1alpha1.Machine) bool {


This naming is not accurate too

aaronfern · 2026-09-09T03:06:17Z

+						machineCurrentStatus := v1alpha1.CurrentStatus{
+							Phase:          v1alpha1.MachineFailed,
+							LastUpdateTime: metav1.Now(),
+						}


Does having this out and assigned to another var help much?

Yes. I update the PreserveExpiryTime below, based on certain criteria. If there is a better way, I can change it.

aaronfern · 2026-09-09T05:08:00Z

 	} else {
-		delete(clone.Annotations, machineutils.PreserveMachineAnnotationKey)
+		klog.V(3).Infof(
+			"Syncing machine %q 's annotation:%q=%q to its node %q 's annotation:%q=%q",


This is worded incorrectly. We sync the node's annotation to the machine

aaronfern · 2026-09-09T05:19:06Z

 					PreserveExpiryTime: machine.Status.CurrentStatus.PreserveExpiryTime,
 				}
+				// check if preservation is needed for the failed machine
+				if val, shouldHandlePreservation := machineutils.GetPreserveAnnotationValue(node, machine); shouldHandlePreservation && val == machineutils.PreserveMachineAnnotationValueWhenFailed {
+					clone.Status.CurrentStatus.PreserveExpiryTime = &metav1.Time{Time: metav1.Now().Add(c.getEffectiveMachinePreserveTimeout(machine).Duration)}
+				}


This snippet could potentially overwrite existing PreserveExpiryTime on the machine that is set a line above

Although, @thiyyakat do you know why we might already have a PreserveExpiryTime on the machine? We enter this block only when the machine is in the Pending phase so I wonder if line 1142 is ever relevant

Correct me if I am wrong @thiyyakat

Only for preserve=now PreserveExpiryTime is set else it is nil. I am setting PreserveExpiryTime only for preserve=when-failed, so they should not conflict.

aaronfern · 2026-09-09T05:20:06Z

 func (c *controller) updateMachineToFailedState(ctx context.Context, description string, machine, clone *v1alpha1.Machine) (bool, error) {
 	// Log the error message for machine failure
 	klog.Error(description)
+	node, _ := c.nodeLister.Get(machine.Annotations[v1alpha1.NodeLabelKey])


Please don't ignore a potential error

After offline discussions, we decided to log the error.

Signed-off-by: r4mek <vivek.ram688@gmail.com>

r4mek requested a review from a team as a code owner September 1, 2026 09:59

gardener-prow Bot added do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 1, 2026

gardener-prow Bot added kind/bug Bug and removed do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Sep 1, 2026

thiyyakat reviewed Sep 3, 2026

View reviewed changes

r4mek marked this pull request as draft September 3, 2026 09:40

gardener-prow Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 3, 2026

r4mek marked this pull request as ready for review September 4, 2026 07:19

gardener-prow Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 4, 2026

gagan16k requested a review from thiyyakat September 6, 2026 16:26

gagan16k reviewed Sep 7, 2026

View reviewed changes

aaronfern reviewed Sep 9, 2026

View reviewed changes

r4mek added 7 commits September 10, 2026 15:09

bugfix

b126218

Signed-off-by: r4mek <vivek.ram688@gmail.com>

make check

d71f726

Signed-off-by: r4mek <vivek.ram688@gmail.com>

introducing PatchMachine()

45a1978

Signed-off-by: r4mek <vivek.ram688@gmail.com>

addressing review comments 1

23cd0cb

Signed-off-by: r4mek <vivek.ram688@gmail.com>

addressing review comments 2

9f05341

Signed-off-by: r4mek <vivek.ram688@gmail.com>

nits

b2503d2

Signed-off-by: r4mek <vivek.ram688@gmail.com>

nits

f28bbed

Signed-off-by: r4mek <vivek.ram688@gmail.com>

r4mek force-pushed the preserve-bug-3 branch from 1fa4282 to f28bbed Compare September 10, 2026 09:41

test fix

dd72aec

Signed-off-by: r4mek <vivek.ram688@gmail.com>

thiyyakat mentioned this pull request Sep 10, 2026

Fix handling of NotFound errors when deciding if a Failed machine should be terminated #1135

Open

		klog.Errorf("Error annotating and setting PreserveExpiryTime on machine %q for auto-preservation: %v", machine.Name, err)
		return nil, err

		// nodeTemplateChanged returns true if machine's nodeTemplate is changed.
		func nodeTemplateChanged(machineset v1alpha1.MachineSet, machine v1alpha1.Machine) bool {

		// configChanged returns true if machine's config is changed.
		func configChanged(machineset v1alpha1.MachineSet, machine v1alpha1.Machine) bool {

		// classKindChanged returns true if machine's class.Kind is changed.
		func classKindChanged(machineset v1alpha1.MachineSet, machine v1alpha1.Machine) bool {

Uh oh!

Conversation

r4mek commented Sep 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gardener-prow Bot commented Sep 1, 2026

Uh oh!

r4mek commented Sep 1, 2026

Uh oh!

thiyyakat left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gagan16k left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aaronfern left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

r4mek commented Sep 1, 2026 •

edited

Loading